chore(deps): update dependency @testing-library/user-event#7840
Conversation
|
Preview: https://patternfly-react-pr-7840.surge.sh A11y report: https://patternfly-react-pr-7840-a11y.surge.sh |
kmcfaul
left a comment
There was a problem hiding this comment.
lgtm. Though there would be conflicts with other open test update PRs with this - should we merge this one first?
| userEvent.type(screen.getByRole('dialog'), '{esc}'); | ||
| expect(props.onClose).toHaveBeenCalled(); | ||
| await user.type(screen.getByRole('dialog'), '{Escape}'); | ||
| waitFor(() => expect(props.onClose).toHaveBeenCalled()); |
There was a problem hiding this comment.
waitFor returns a promise which should be awaited. There are several other waitFor usages in this change that should be awaited as well.
|
@kmcfaul we could, though I think it might make more sense to delay the merging of this until the start of the next milestone. I don't mind resolving the conflicts and making the updates needed on this PR, and I wouldn't want to delay all of the other test-changing PRs and potentially delay code freeze. I'm definitely open to alternative approaches though, if you think it would be better to go ahead and merge this sooner rather than later I won't object. |
|
I'm reverting this to a draft as I've found some issues with tests that shouldn't be passing. |
c12fd83 to
895eed1
Compare
|
Biggest changes since the first time I put this up as ready for review: I had to replace our usage of In some of the DropDown integration tests (in 1a2dc60) I also had to add an additional Other small changes needed were changing special keys in userEvent.type/.keyboard to match their new API for those keys. |
|
|
||
| await user.click(input); | ||
|
|
||
| // fireEvent is used here do to an issue with the current version userEvent |
There was a problem hiding this comment.
due* to an issue (same as other instances of this comment).
Is there a github issue we can link here pointing to the issue or can the issue be described here in more detail?
There was a problem hiding this comment.
No github issue that I'm aware of, a test just started failing all of a sudden in a release version that shouldn't have been breaking. I'll explain some of that in the comment and fix the due/do flub in just a moment.
There was a problem hiding this comment.
Also I'll rebase once more and fix the merge conflict
ec45fc1 to
9cc095e
Compare
| render(<AboutModal {...props} isOpen />); | ||
|
|
||
| userEvent.type(screen.getByRole('dialog'), '{esc}'); | ||
| await user.type(screen.getByRole('dialog'), '{Escape}'); |
There was a problem hiding this comment.
Are strings like Escape here something we should be using enums or constants for? So that we don't have to update all these values ever again? And can reuse them in the future? just an idea.
|
|
||
| onEnterPressed = (event: any) => { | ||
| if (event.charCode === KEY_CODES.ENTER) { | ||
| if (event.key === KeyTypes.Enter) { |
There was a problem hiding this comment.
oh - maybe this is the constant I was imagining. Could we uses these/expand these everywhere you have hardcoded KeyTypes?
There was a problem hiding this comment.
Yeah I really like that idea!
Since RTL requires those {} around the actual key value though I would either need to either place the KeyTypes value in a template string each time they're used or create a new constant for RTL keys which does so, which approach do you think would be better?
There was a problem hiding this comment.
if the use of the template string means that we'd have one 'gold source' for key values, then I think it'd opt for that.
…y#7840) * chore(deps): update dependency @testing-library/user-event * fix AboutModal * fix Modal * fix other breaking changes * await transformers test helper assertion * update key press events * further transition from using .keyCode to .key * fix dropdown integration tests * revert some usage of .trigger to .type * rebase * resolve failing test in user-event v14.4.3 * improve fireEvent usage explanation * update tests to use KeyTypes constant * add missed KeyTypes
What: Closes #7839
Additional issues:
Breaking changes introduced in the newer release are listed here: https://github.com/testing-library/user-event/releases/tag/v14.0.0
One additional change I made in this PR that isn't technically breaking is the addition of
userEvent.setup(), because the RTL team has stated that is the preferred way to useuserEventgoing forward here: https://testing-library.com/docs/user-event/intro#writing-tests-with-userevent